-
Notifications
You must be signed in to change notification settings - Fork 795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize reflection of F# types, part 2 #9784
Conversation
…teRecordFieldReader
(fun (obj: obj) -> | ||
let m2b = typ.GetMethod("GetTag", BindingFlags.Static ||| bindingFlags, null, [| typ |], null) | ||
m2b.Invoke(null, [|obj|]) :?> int) | ||
let m2b = typ.GetMethod("GetTag", BindingFlags.Static ||| bindingFlags, null, [| typ |], null) | ||
(fun (obj: obj) -> m2b.Invoke(null, [|obj|]) :?> int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably no need to look up the method on every invocation. I didn't know how to test this specifically though. When is it ever the case that a DU doesn't have a Tag
property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A DU always has a tag as far as I know - struct/ref type, single case/multi case all have tag properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, then I'm not sure if this branch executes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Paul had PR to do tagless DUs a few (lot of) years ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Thank you for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if there was a way to further reduce allocations/CPU time by making the loop not call into an enumerator, but that's clearly a micro-optimization at this point.
@cartermp, what loop? The only loops I see are those that are used to create expression trees during the precomputation. There are no loops inside the compiled delegates. |
In this case I'm referring to a loop like this: https://github.com/dotnet/fsharp/pull/9784/files#diff-54378f53a8612b2adf50a6efb115ba5aR134 The compiler rewrites this to use an enumerator no matter which kind of current impl Rewriting it to a while loop and manually bumping the indexer will remove that like so but it's unlikely to matter much from a perf standpoint. Just kind of weird that trivially changing the |
Yeah, I'd be all for altering the loop were this happening inside of the delegate. Don't think it matters at all during prep stage. |
In hot paths it matters for ints, it's about 4x slower when it becomes an enumeration. That may not matter much here, though. There's an issue open to better optimize this, the example from @cartermp will help there. |
This func is compileUnionCaseConstructorFunc ... it should not happen frequently. |
@kerams if you want to further improve performance I have a method added to your benchmark from the previous PR and results laying around at home that I can post next week if it is of interest. and it might be a flaw in the benchmark code |
@Daniel-Svensson, are you saying var a = int[5];
a[0] = 0;
a[1] = 0;
a[2] = 0;
a[3] = 0;
a[4] = 0; is 6 times slower than var a = int[5];
a[4] = 0;
a[3] = 0;
a[2] = 0;
a[1] = 0;
a[0] = 0; ?? Apologies for using 'the other sharp'. |
@kerams actually I belive I must have a bug in my fsharp (cannot check it this week), it was a quick hack late before bedtime. |
* Compile PreComputeRecordConstructor, PreComputeRecordReader, PreComputeRecordFieldReader * Compile PreComputeUnionConstructor, PreComputeUnionTagReader, PreComputeUnionReader
Continuation of #9714.
PreComputeRecordConstructor
PreComputeRecordFieldReader
PreComputeRecordReader
- reworked from last timePreComputeUnionConstructor
PreComputeUnionReader
PreComputeUnionTagReader
PreComputeTupleReader
andPreComputeTupleConstructor
are a little more complicated, so perhaps another time.The rest of the
PreCompute
family only return correspondingSystem.Reflection.MethodBase
for further reflection use.